fix(workspace): deletion must take the agents' data with it - #187
fix(workspace): deletion must take the agents' data with it#187WhichPaths wants to merge 1 commit into
Conversation
|
Thanks — the bug is real and the integration tests here are the best coverage anyone has written for the purge path. I'm not merging this one as-is, though, because it now collides with #207, which I merged a few minutes ago. #207 fixes the same two touch points:
#207 also covered ground this PR doesn't: What this PR still has that #207 does not, and what I'd like to keep:
Could you rebase onto |
Rebased onto main and reduced to what yetone#207 did not cover, per review. Dropped: the fs-endpoints.ts change. yetone#207 landed it, and its version is better — the tenant comes from c.companyId (the JWT claim the endpoints already 403 on when empty) rather than a subquery against participants, so it is one round trip fewer and cannot write NULL when the participant row is missing. The test that exercised that endpoint went with it; yetone#207 asserts the same property in runtime-server.test.ts. Kept, and the only behaviour change here: agent_events, agent_runs and agent_triages join the by-owner sweep, taking it from five tables to eight. Worth being precise about why, because it is not the same reason as the five. Those five were leaking: their writers were dropping company_id, so the tenant sweep could not see the rows. These three have writers that all pass a tenant today — I checked every createRun, recordEvent and recordTriage call site — so nothing is leaking through them right now. They are here so the sweep stops depending on writer discipline at all, which is the property that failed for the other five. And they are the tables carrying per-run history and cost, so a row that does slip through resurfaces on a billing report rather than in a UI. Six of the seven integration tests pass against main's five-table list; the seventh is the one that pins the three, and it seeds them the way a writer that forgot the tenant would. Two are guards in the other direction: a correctly-tenanted row is still removed, and another workspace keeps its own run history.
d506929 to
f638290
Compare
|
Rebased onto Dropped: the Kept: the widened sweep. One correction to the framing, because I'd rather you merged this knowing it: These three are not leaking today. The five in #207 were — their writers were dropping That shows up honestly in the tests: six of the seven pass against main's five-table list. Only one fails, and it is the one that seeds those three the way a writer that forgot the tenant would: Two of the seven are guards in the other direction — a correctly-tenanted row must still be removed (the tenant sweep is not being replaced), and another workspace must keep its own run history (deleting by owner must not reach past the workspace being deleted). Checks: Final diff is 7 lines in |
2 participants
Deleting a workspace leaves the agents' memory and their written notes about people behind.
The purge sweeps its soft-scoped tables with
which is exactly as good as the
company_idthe writers put there. Two of them do not put one.agent_workspace— the agent's own filesystem endpoint writes no tenant:Every other writer of that table does —
skills.ts:262,cli.ts:2713/4116/4122/4363,router.ts:2962, and the baseline backfill atmigrate.ts:647— and they refresh it on conflict too. This is the one path that doesn't, and it is the path an agent uses to write its own memory.agent_climate— the column isTEXT NOT NULL DEFAULT 'personal'(migrate.ts:822) and neither of its two INSERT sites (climate.ts:57,cli.ts:4250) names it. So every climate row in every workspace is labelled'personal', andDELETE FROM agent_climate WHERE company_id = 'co-acme'can never match one.Demonstrated
Postgres 16, one agent in
co-tenant, one row written the FUSE way and one written the way a peer writer does:Same agent, same table, same deletion — one row goes, one stays. The one that stays is the memory file.
The fix, in two halves
Stop producing them.
fs-endpointsfills the tenant from the agent'sparticipantsrow with a scalar subquery, so it stays a single round trip and still inserts when the lookup finds nothing — behaviour is unchanged where it was already correct. TheON CONFLICTarm usesCOALESCE(EXCLUDED.company_id, agent_workspace.company_id), so a row already written without one heals on the agent's next write and a known tenant is never overwritten with NULL.Reach the ones already written. The purge also sweeps the agent-owned subset by
agent_id:agentIdsis already built earlier in the same transaction forboard_mention_reads. Deleting by owner needs no backfill migration to reach rows written before this, which matters because those rows exist in every deployment today.computersis deliberately not in the subset — it is keyed by the machine, not an agent. I confirmed against the live schema that all eight tables in the subset really do have anagent_idcolumn.Why the existing test didn't catch it
workspace-management.test.ts's purge test seeds every row with a correctcompany_id:`INSERT INTO agent_runs (id, agent_id, company_id) VALUES ('run-managed', 'agent-managed', 'co-managed')`That is the one shape the broken writers never produce, so the test passes on the broken code. The new tests seed the way the real writers do.
Verification
fs-endpointschange → test 5 fails (the row is written without a workspace)PUT /runtime/fs/writewith a minted agent token rather than issuing the SQL itself, so it pins the writer, not my restatement of it.company_idsweep already reached is still removed, and another workspace's agent keeps its data — deleting by owner must not reach past the workspace being deleted.workspace-management.test.tsstays green, 12/12.tsc --noEmit,biome lint ., all three source guards clean.Related, not fixed here
agent_climate's writers should probably namecompany_idrather than relying on a'personal'default that is wrong for every non-personal workspace — the column and its index (idx_agent_climate_company) exist as if it were meaningful, andAGENT_ID_CASCADE_TABLESscopes on it. But climate reads are per-agent and global by design (ADR 0004 says so explicitly), so changing what the column holds is a separate decision with its own blast radius. This PR makes deletion correct without taking that on.